[WEB-3268] feat: url pattern #6546
Conversation
anmolsinghbhatia
added
🌟enhancement
WalkthroughThis PR introduces two new Django API endpoints for retrieving issue metadata and details using unique identifiers. On the frontend, new and modified React components handle issue detail views and redirection logic. Numerous UI components have been refactored to use a new helper function for generating work item links, ensuring uniform URL construction across the application. Additionally, the issue service and store have been enhanced to support fetching issues by identifier, along with minor updates to utility functions and permission checks. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Django Router
participant V as IssueMetaEndpoint
participant P as Permission Checker
participant DB as Database
C->>R: Request /workspaces/{slug}/projects/{project_id}/issues/{issue_id}/meta/
R->>V: Route request to IssueMetaEndpoint
V->>P: Check user permissions
P-->>V: Return permission status
V->>DB: Query issue metadata
DB-->>V: Return {project_identifier, sequence_id}
V-->>C: Return issue metadata JSON
sequenceDiagram
participant U as User
participant UI as IssueDetailsPage
participant S as IssueService
participant Router as Next.js Router
U->>UI: Navigate to issue detail page
UI->>S: fetchIssueWithIdentifier(slug, project_identifier, sequence_id)
S-->>UI: Return issue data or metadata
UI->>Router: Redirect using generated workItemLink
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx (1)
201-206: Use the new helper function for copying issue link in the menu.For consistency, the menu's copy link functionality should also use the
generateWorkItemLinkhelper.Apply this diff to update the copy link functionality:
- subIssueOperations.copyText(`${workspaceSlug}/projects/${issue.project_id}/issues/${issue.id}`); + subIssueOperations.copyText(workItemLink);web/core/components/issues/sub-issues/issue-list-item.tsx (1)
201-206: Use the new helper function for copying issue link in the menu.For consistency, the menu's copy link functionality should also use the
generateWorkItemLinkhelper.Apply this diff to update the copy link functionality:
- subIssueOperations.copyText(`${workspaceSlug}/projects/${issue.project_id}/issues/${issue.id}`); + subIssueOperations.copyText(workItemLink);
🧹 Nitpick comments (12)
web/app/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx (1)
34-78: Optimize breadcrumbs rendering to prevent unnecessary re-renders.The breadcrumbs component contains nested conditional rendering and complex logic. Consider extracting the breadcrumbs into a separate memoized component to optimize performance.
+const ProjectBreadcrumbs = memo(({ projectDetails, workspaceSlug, projectId, issueDetails }) => ( + <Breadcrumbs.BreadcrumbItem + type="text" + link={ + <BreadcrumbLink + label={projectDetails?.name ?? "Project"} + icon={ + projectDetails ? ( + <span className="grid place-items-center flex-shrink-0 h-4 w-4"> + <Logo logo={projectDetails?.logo_props} size={16} /> + </span> + ) : ( + <span className="grid h-7 w-7 flex-shrink-0 place-items-center rounded uppercase"> + <Briefcase className="h-4 w-4" /> + </span> + ) + } + /> + } + /> +));web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx (1)
56-68: Consider debouncing the resize event handler.The resize event handler could fire frequently during window resizing. Consider debouncing the handler to improve performance.
+import { debounce } from "lodash"; + useEffect(() => { - const handleToggleIssueDetailSidebar = () => { + const handleToggleIssueDetailSidebar = debounce(() => { if (window && window.innerWidth < 768) { toggleIssueDetailSidebar(true); } if (window && issueDetailSidebarCollapsed && window.innerWidth >= 768) { toggleIssueDetailSidebar(false); } - }; + }, 250); window.addEventListener("resize", handleToggleIssueDetailSidebar); handleToggleIssueDetailSidebar(); return () => window.removeEventListener("resize", handleToggleIssueDetailSidebar); }, [issueDetailSidebarCollapsed, toggleIssueDetailSidebar]);web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx (1)
65-73: Consider adding error handling for link operations.While the implementation is good, consider adding error handling for failed copy operations.
const handleCopyIssueLink = () => - copyUrlToClipboard(workItemLink, false).then(() => + copyUrlToClipboard(workItemLink, false) + .then(() => setToast({ type: TOAST_TYPE.SUCCESS, title: "Link copied", message: "Issue link copied to clipboard", }) + ) + .catch(() => + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error", + message: "Failed to copy link to clipboard", + }) + );web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx (1)
65-71: Add null checks for optional parameters.The
generateWorkItemLinkfunction is called with optional parameters that could be undefined. Consider adding null checks or providing default values.const workItemLink = generateWorkItemLink({ workspaceSlug: workspaceSlug.toString(), - projectId: issue?.project_id, + projectId: issue?.project_id ?? "", issueId: issue?.id, - projectIdentifier, + projectIdentifier: projectIdentifier ?? "", - sequenceId: issue?.sequence_id, + sequenceId: issue?.sequence_id ?? 0, });web/core/components/issues/relations/issue-list-item.tsx (2)
66-73: Add null checks for optional parameters and consider extracting link generation.
- Similar to other components, add null checks for optional parameters.
- Consider extracting the link generation to a useMemo hook to avoid regenerating on every render.
+const workItemLink = useMemo(() => generateWorkItemLink({ workspaceSlug: workspaceSlug.toString(), - projectId: issue?.project_id, + projectId: issue?.project_id ?? "", issueId: issue?.id, - projectIdentifier: projectDetail?.identifier, + projectIdentifier: projectDetail?.identifier ?? "", - sequenceId: issue?.sequence_id, + sequenceId: issue?.sequence_id ?? 0, isEpic: issue?.is_epic, - }); + }), + [workspaceSlug, issue?.project_id, issue?.id, projectDetail?.identifier, issue?.sequence_id, issue?.is_epic] +);
76-83: Consider extracting the epic link handling logic.The epic link handling logic in
handleIssuePeekOverviewcould be extracted to a separate function for better maintainability.+const handleEpicLink = (issue: TIssue) => { + window.open(workItemLink, "_blank"); +}; const handleIssuePeekOverview = (issue: TIssue) => { if (issue.is_epic) { - window.open(workItemLink, "_blank"); + handleEpicLink(issue); return; } handleRedirection(workspaceSlug, issue, isMobile); };web/helpers/issue.helper.ts (1)
329-351: LGTM! Well-structured helper function with comprehensive type safety.The function effectively centralizes URL generation logic and handles different cases appropriately. Consider adding JSDoc comments to document the function's purpose and parameters.
+/** + * Generates a URL for a work item based on its properties. + * @param workspaceSlug - The workspace identifier + * @param projectId - The project's unique identifier + * @param issueId - The issue's unique identifier + * @param projectIdentifier - The project's display identifier + * @param sequenceId - The issue's sequence number + * @param isArchived - Whether the issue is archived + * @param isEpic - Whether the issue is an epic + * @returns A URL string for the work item + */ export const generateWorkItemLink = ({ workspaceSlug, projectId, issueId,web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx (1)
197-203: Consider extracting the URL generation logic to reduce duplication.The URL generation setup is repeated in
CreatedUpcomingIssueListItem,CreatedOverdueIssueListItem, andCreatedCompletedIssueListItem. Consider extracting this into a shared helper function.+const getWorkItemLinkFromIssue = ( + workspaceSlug: string, + issue: TIssue, + projectDetails: any +) => + generateWorkItemLink({ + workspaceSlug, + projectId: issue?.project_id, + issueId: issue?.id, + projectIdentifier: projectDetails?.identifier, + sequenceId: issue?.sequence_id, + }); + export const CreatedUpcomingIssueListItem: React.FC<IssueListItemProps> = observer((props) => { // ... - const workItemLink = generateWorkItemLink({ - workspaceSlug, - projectId: issue?.project_id, - issueId: issue?.id, - projectIdentifier: projectDetails?.identifier, - sequenceId: issue?.sequence_id, - }); + const workItemLink = getWorkItemLinkFromIssue(workspaceSlug, issue, projectDetails);Also applies to: 265-271, 331-337
apiserver/plane/app/views/issue/base.py (2)
1117-1273: Consider improving error handling and response codes.While the implementation is generally good, there are a few areas that could be improved:
- The HTTP status code for guest user access denial should be 403 (Forbidden) instead of 400 (Bad Request) at line 1260.
- Consider adding input validation for
project_identifierandissue_identifierparameters.Apply this diff to improve the error handling:
- status=status.HTTP_400_BAD_REQUEST, + status=status.HTTP_403_FORBIDDEN,Additionally, consider adding input validation at the start of the method:
def get(self, request, slug, project_identifier, issue_identifier): # Validate input parameters if not project_identifier or not issue_identifier: return Response( {"error": "Project identifier and issue identifier are required"}, status=status.HTTP_400_BAD_REQUEST, ) try: issue_identifier = int(issue_identifier) except ValueError: return Response( {"error": "Invalid issue identifier"}, status=status.HTTP_400_BAD_REQUEST, )
1228-1228: Fix indentation in SQL query.The indentation in the query filter is inconsistent with the surrounding code.
Apply this diff to fix the indentation:
- issue__sequence_id=issue_identifier, + issue__sequence_id=issue_identifier,web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx (2)
13-13: Consider reusing or injecting service instances.
IfIssueServiceholds no state, this is acceptable. Otherwise, for testability and to avoid multiple instantiations, consider using a shared or injected service.
38-40: Add accessibility support for loading indicator.
Consider adding a suitable ARIA label or screen-reader-friendly fallback to ensure users relying on assistive technology understand the page is loading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
apiserver/plane/app/urls/issue.py(2 hunks)apiserver/plane/app/views/__init__.py(1 hunks)apiserver/plane/app/views/issue/base.py(1 hunks)web/app/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/header.tsx(0 hunks)web/core/components/core/activity.tsx(3 hunks)web/core/components/core/modals/existing-issues-list-modal.tsx(2 hunks)web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx(4 hunks)web/core/components/inbox/content/inbox-issue-header.tsx(4 hunks)web/core/components/inbox/content/inbox-issue-mobile-header.tsx(3 hunks)web/core/components/inbox/content/issue-properties.tsx(3 hunks)web/core/components/issues/create-issue-toast-action-items.tsx(3 hunks)web/core/components/issues/issue-detail-widgets/relations/helper.tsx(1 hunks)web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx(4 hunks)web/core/components/issues/issue-detail/root.tsx(1 hunks)web/core/components/issues/issue-layouts/calendar/issue-block.tsx(3 hunks)web/core/components/issues/issue-layouts/gantt/blocks.tsx(2 hunks)web/core/components/issues/issue-layouts/kanban/block.tsx(4 hunks)web/core/components/issues/issue-layouts/list/block.tsx(2 hunks)web/core/components/issues/issue-layouts/properties/all-properties.tsx(2 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx(2 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/cycle-issue.tsx(3 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/module-issue.tsx(3 hunks)web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx(3 hunks)web/core/components/issues/issue-layouts/spreadsheet/issue-row.tsx(4 hunks)web/core/components/issues/parent-issues-list-modal.tsx(2 hunks)web/core/components/issues/peek-overview/header.tsx(3 hunks)web/core/components/issues/relations/issue-list-item.tsx(4 hunks)web/core/components/issues/sub-issues/issue-list-item.tsx(2 hunks)web/core/hooks/use-issue-peek-overview-redirection.tsx(3 hunks)web/core/services/issue/issue.service.ts(1 hunks)web/core/store/issue/issue-details/issue.store.ts(3 hunks)web/core/store/issue/issue-details/root.store.ts(1 hunks)web/core/store/issue/issue.store.ts(3 hunks)web/helpers/issue.helper.ts(1 hunks)web/helpers/string.helper.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/header.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/store/issue/issue-details/issue.store.ts
[error] 311-311: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (59)
web/core/hooks/use-issue-peek-overview-redirection.tsx (6)
6-6: Helper import is correct.
9-9: Multiple hooks import is fine.
18-18:useProject()usage looks good.
Be aware of scenarios where project data might not exist yet.
28-28: Validate project identifier existence.
getProjectIdentifierById(issue?.project_id)could return undefined if the project is missing; ensure upstream logic handles that case.
30-38: Link generation approach is clean and consistent.
UsinggenerateWorkItemLinkcentralizes link logic effectively.
41-41: Good straightforward redirection.
No issues found.web/core/components/issues/create-issue-toast-action-items.tsx (7)
5-5: ImportinggenerateWorkItemLinkis appropriate.
8-8: Combined hook imports are fine.
25-25:getProjectIdentifierByIdusage is okay.
Confirm that it gracefully handles missing projects.
29-29: Double-check for undefined project identifiers.
If no matching project is found, this may return undefined.
33-40: Centralized link generation is good practice.
Having all link logic in one helper keeps things consistent across the codebase.
44-44: Clipboard usage is straightforward.
No issues found; you already handle the copy result.
57-57: Correctly assigninghrefto the generated link.web/app/[workspaceSlug]/(projects)/browse/[workItem]/header.tsx (1)
25-28: Consider adding error handling for undefined project details.The code assumes that
getProjectByIdwill always return valid data whenprojectIdis defined. Add error handling to gracefully handle cases where project details might be undefined.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if getProjectById is used elsewhere with error handling ast-grep --pattern 'getProjectById($_)'Length of output: 10473
Action: Confirm consistent handling of undefined project details
The code in header.tsx mirrors the widely used pattern in the codebase—invoking
getProjectByIdconditionally on the existence ofprojectIdand assigning its result (which may beundefined) directly. We see similar usage with no extra error checks in many other files. This indicates that handling of a potentially undefined result is either done later in the UI or intentionally deferred.• The current implementation is consistent with the rest of the project.
• No additional immediate error handling is warranted specifically in header.tsx.web/core/components/issues/issue-detail-widgets/relations/helper.tsx (1)
33-33: LGTM! URL construction aligned with new pattern.The removal of the extra forward slash aligns with the PR objectives to implement a shorter URL format.
web/core/components/issues/issue-layouts/gantt/blocks.tsx (1)
109-116: LGTM! Consistent URL generation implementation.The use of
generateWorkItemLinkhelper ensures consistent URL construction across the application, aligning with the PR objectives.web/core/store/issue/issue.store.ts (3)
17-22: LGTM! Well-structured type definitions.The new
issuesIdentifierMapand its related methods are well-defined with proper TypeScript types.
74-79: LGTM! Robust implementation of addIssueIdentifier.The method includes proper null checks and uses MobX's
runInActionfor state updates.
143-146: LGTM! Efficient implementation of getIssueIdByIdentifier.The computed function is properly memoized using
computedFnand includes necessary null checks.apiserver/plane/app/views/__init__.py (1)
119-120: LGTM! Clean addition of new endpoints.The new endpoints are properly imported following the existing pattern.
web/core/components/issues/issue-layouts/calendar/issue-block.tsx (3)
16-18: LGTM! Clean import structure.The new helper and hooks are properly imported.
78-86: LGTM! Comprehensive link generation.The
generateWorkItemLinkis called with all necessary parameters including handling of epics and archived items.
54-54: Verify project identifier retrieval.Ensure that
getProjectIdentifierByIdalways returns a value for valid project IDs.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for getProjectIdentifierById implementation and its error handling ast-grep --pattern 'getProjectIdentifierById($$$) { $$$ }'Length of output: 62
Let's gather more information on the implementation of
getProjectIdentifierById. We'll search for its definition usingrgto see how it's implemented and whether it always returns a value for valid project IDs.
🏁 Script executed:
#!/bin/bash echo "Searching for the definition of getProjectIdentifierById..." rg -n -e "export function getProjectIdentifierById" . rg -n -e "const getProjectIdentifierById\\s*=" .Length of output: 234
Let's run an additional script to search for all references to
getProjectIdentifierById(including its definition and import statements) throughout the codebase to better understand its implementation and usage.
🏁 Script executed:
#!/bin/bash echo "Searching for all occurrences of getProjectIdentifierById in the repository..." rg -n "getProjectIdentifierById" . echo "" echo "Searching for its definition patterns (function and constant declarations)..." rg -n "function getProjectIdentifierById" . rg -n "const getProjectIdentifierById" . echo "" echo "Showing import lines in issue-block.tsx for additional context..." rg -n "import.*getProjectIdentifierById" web/core/components/issues/issue-layouts/calendar/issue-block.tsxLength of output: 5521
Let's extract the implementation of
getProjectIdentifierByIdfrom the project store to inspect its error and fallback handling. We'll review the surrounding context to understand if the function always returns a value for valid project IDs.
🏁 Script executed:
#!/bin/bash echo "Extracting getProjectIdentifierById definition from web/core/store/project/project.store.ts..." sed -n '370,410p' web/core/store/project/project.store.tsLength of output: 1632
Review Verification:
getProjectIdentifierByIdin the Project Store
- The implementation in
web/core/store/project/project.store.tsretrieves the project information fromprojectMapusing the providedprojectId(defaulting to the empty string ifprojectIdis nullish).- It returns the
identifierproperty from the fetched project info. For valid project IDs—that is, those with corresponding entries inprojectMap—this results in the expected identifier.- There is no explicit fallback or error handling for cases where a project is not found; however, this behavior is consistent with its usage in the codebase where it is assumed that valid IDs are always present in the store.
Based on the available evidence, for valid project IDs the function returns the expected identifier value.
web/core/components/issues/issue-layouts/quick-action-dropdowns/all-issue.tsx (2)
19-22: LGTM! Clean import structure.The new helper and hooks are properly imported.
57-63: LGTM! Proper link generation.The
generateWorkItemLinkis called with all necessary parameters.web/core/components/issues/issue-layouts/quick-action-dropdowns/project-issue.tsx (1)
69-71: Similar issue with null checks as in module-issue.tsx.The same concerns about handling undefined values apply here.
web/core/components/issues/issue-layouts/quick-action-dropdowns/cycle-issue.tsx (1)
65-71: Similar issue with null checks as in other quick action components.The same concerns about handling undefined values apply here.
web/core/components/issues/peek-overview/header.tsx (4)
25-25: LGTM! Imports are well organized.The imports for
generateWorkItemLinkanduseProjecthook are correctly placed in their respective sections.Also applies to: 28-28
90-90: LGTM! Project identifier retrieval is correctly implemented.The
getProjectIdentifierByIdhook is properly used to get the project identifier for the new URL format.Also applies to: 95-95
97-104: LGTM! Work item link generation is properly implemented.The
generateWorkItemLinkfunction is correctly used with all required parameters for the new URL format.
109-109: LGTM! Link usage is consistent.The generated work item link is correctly used in the copy to clipboard functionality and navigation.
Also applies to: 136-136
web/core/components/inbox/content/inbox-issue-mobile-header.tsx (4)
26-26: LGTM! Imports are well organized.The imports for
generateWorkItemLinkanduseProjecthook are correctly placed in their respective sections.Also applies to: 28-28
82-82: LGTM! Project identifier retrieval is correctly implemented.The
getProjectIdentifierByIdhook is properly used to get the project identifier for the new URL format.Also applies to: 91-91
93-99: LGTM! Work item link generation is properly implemented.The
generateWorkItemLinkfunction is correctly used with all required parameters for the new URL format.
149-149: LGTM! Link usage is consistent.The generated work item link is correctly used in the router navigation.
Also applies to: 183-183
web/core/components/inbox/content/issue-properties.tsx (3)
13-13: LGTM! Import is well organized.The import for
generateWorkItemLinkis correctly placed in the helper section.
38-44: LGTM! Work item link generation for duplicate issues is properly implemented.The
generateWorkItemLinkfunction is correctly used with all required parameters for the duplicate issue URL format.
181-184: LGTM! Link usage is consistent.The generated work item link is correctly used in both the href and router navigation.
web/helpers/string.helper.ts (1)
71-71: LGTM! URL formatting flexibility is enhanced.The addition of the
addSlashparameter with a default value oftruemaintains backward compatibility while providing flexibility for URL formatting.Also applies to: 76-76, 79-79
web/core/components/issues/parent-issues-list-modal.tsx (1)
195-202: LGTM! URL generation is now consistent.The implementation correctly uses the
generateWorkItemLinkhelper function with all required parameters, aligning with the PR's objective of standardizing URL patterns.web/core/components/issues/issue-layouts/kanban/block.tsx (1)
154-162: LGTM! Comprehensive URL generation with proper handling of epics and archived items.The implementation correctly uses the
generateWorkItemLinkhelper function with all required parameters, including proper handling of epics and archived items.web/core/components/issues/issue-detail/issue-detail-quick-actions.tsx (1)
76-82: LGTM! URL generation is now consistent.The implementation correctly uses the
generateWorkItemLinkhelper function with all required parameters.web/core/components/issues/sub-issues/issue-list-item.tsx (1)
88-94: LGTM! URL generation is now consistent.The implementation correctly uses the
generateWorkItemLinkhelper function with all required parameters.apiserver/plane/app/urls/issue.py (1)
283-292: LGTM! The new URL patterns are well-structured.The new endpoints for issue metadata and issue details follow RESTful conventions and maintain consistency with existing patterns.
web/core/components/issues/issue-layouts/list/block.tsx (1)
153-161: LGTM! Good use of the new URL generation helper.The implementation correctly uses
generateWorkItemLinkwith all required parameters, ensuring consistent URL generation across the application.web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx (1)
197-203: LGTM! Consistent use of the URL generation helper across components.The implementation correctly uses
generateWorkItemLinkwith all required parameters in each component.Also applies to: 265-271, 331-337
web/core/components/core/modals/existing-issues-list-modal.tsx (1)
273-279: LGTM! Good use of the helper function.The change improves code maintainability by using the centralized
generateWorkItemLinkhelper function.web/core/components/issues/issue-detail/root.tsx (1)
335-340: LGTM! Enhanced permission check with proper context.The change improves security by making the permission check context-aware with workspace and project parameters.
web/core/components/issues/issue-layouts/spreadsheet/issue-row.tsx (1)
246-254: LGTM! Good use of the helper function.The change improves code maintainability by using the centralized
generateWorkItemLinkhelper function with proper parameter handling.web/core/services/issue/issue.service.ts (1)
443-456: LGTM! The new methods follow the established patterns.Both methods are well-implemented with proper error handling and consistent patterns:
getIssueMetaFromURL: Follows RESTful API pattern and returns strongly typed response.retrieveWithIdentifier: Reuses existing logic for handling epics and local updates.Also applies to: 458-481
web/core/components/inbox/content/inbox-issue-header.tsx (1)
230-236: LGTM! Improved maintainability by centralizing URL generation.The changes enhance code maintainability by:
- Using a dedicated helper function for URL generation
- Ensuring consistent URL structure across the application
- Making it easier to update URL patterns in the future
Also applies to: 365-366, 369-369, 439-439
web/core/store/issue/issue-details/root.store.ts (1)
262-263: LGTM! The new method follows established patterns.The
fetchIssueWithIdentifiermethod:
- Follows the established pattern of delegating to the underlying store
- Maintains proper type safety
- Enhances the API surface with a new way to fetch issues
web/core/components/issues/issue-layouts/properties/all-properties.tsx (1)
246-256: LGTM! Enhanced link generation with proper parameter handling.The changes improve the code by:
- Using the centralized URL generation helper
- Properly handling all required parameters including archived and epic states
- Ensuring consistent URL structure across the application
web/core/components/core/activity.tsx (2)
27-27: LGTM! Good addition of the helper function import.The import of
generateWorkItemLinkaligns with the PR objectives for consistent link generation.
38-44: LGTM! Well-structured link generation implementation.The implementation correctly uses the new helper function with all required parameters for generating consistent work item links.
apiserver/plane/app/views/issue/base.py (1)
1101-1114: LGTM! Well-implemented metadata endpoint.The
IssueMetaEndpointis cleanly implemented with proper permission checks and returns only the necessary fields for issue identification.web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx (3)
3-3: No issues found.
The use ofuseEffectis standard for managing side effects in React components.
7-7: Minor check on loading indicator usage.
ImportingLogoSpinneris fine. However, consider confirming the user experience for accessibility or fallback messaging, especially on slower connections.
10-11: Validate service viability in client environment.
The import path looks consistent. IfIssueServiceinternally depends on Node APIs, confirm it’s safe for client usage in Next.js.
| ? () => fetchIssueWithIdentifier(workspaceSlug.toString(), projectIdentifier, sequence_id) | ||
| : null | ||
| ); | ||
| console.log("data", data); |
There was a problem hiding this comment.
Remove debug console.log statement.
Remove the console.log statement before merging to production.
- console.log("data", data);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("data", data); |
| if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions); | ||
| if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link); | ||
| if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments); | ||
| this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed); |
There was a problem hiding this comment.
Remove duplicate code block.
The code block for handling related data (reactions, links, attachments, subscriptions) is duplicated within the same method.
Remove the duplicate block:
- // add related data
- if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions);
- if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link);
- if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments);
- this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed);
// fetch related data
// issue reactions
if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issueId, issue.issue_reactions);
// fetch issue links
if (issue.issue_link) this.rootIssueDetailStore.addLinks(issueId, issue.issue_link);
// fetch issue attachments
if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issueId, issue.issue_attachments);
this.rootIssueDetailStore.addSubscription(issueId, issue.is_subscribed);Also applies to: 328-337
| fetchIssueWithIdentifier = async (workspaceSlug: string, project_identifier: string, sequence_id: string) => { | ||
| const query = { | ||
| expand: "issue_reactions,issue_attachments,issue_link,parent", | ||
| }; | ||
| const issue = await this.issueService.retrieveWithIdentifier(workspaceSlug, project_identifier, sequence_id, query); | ||
| const issueIdentifier = `${project_identifier}-${sequence_id}`; | ||
| const issueId = issue?.id; | ||
| const projectId = issue?.project_id; | ||
|
|
||
| if (!issue || !projectId || !issueId) throw new Error("Issue not found"); | ||
|
|
||
| const issuePayload = this.addIssueToStore(issue); | ||
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]); | ||
|
|
||
| // handle parent issue if exists | ||
| if (issue?.parent && issue?.parent?.id && issue?.parent?.project_id) { | ||
| this.issueService.retrieve(workspaceSlug, issue.parent.project_id, issue.parent.id).then((res) => { | ||
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([res]); | ||
| }); | ||
| } | ||
|
|
||
| // add identifiers to map | ||
| this.rootIssueDetailStore.rootIssueStore.issues.addIssueIdentifier(issueIdentifier, issueId); | ||
|
|
||
| // add related data | ||
| if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions); | ||
| if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link); | ||
| if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments); | ||
| this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed); | ||
|
|
||
| // fetch related data | ||
| // issue reactions | ||
| if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issueId, issue.issue_reactions); | ||
|
|
||
| // fetch issue links | ||
| if (issue.issue_link) this.rootIssueDetailStore.addLinks(issueId, issue.issue_link); | ||
|
|
||
| // fetch issue attachments | ||
| if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issueId, issue.issue_attachments); | ||
|
|
||
| this.rootIssueDetailStore.addSubscription(issueId, issue.is_subscribed); | ||
|
|
||
| // fetch issue activity | ||
| this.rootIssueDetailStore.activity.fetchActivities(workspaceSlug, projectId, issueId); | ||
|
|
||
| // fetch issue comments | ||
| this.rootIssueDetailStore.comment.fetchComments(workspaceSlug, projectId, issueId); | ||
|
|
||
| // fetch sub issues | ||
| this.rootIssueDetailStore.subIssues.fetchSubIssues(workspaceSlug, projectId, issueId); | ||
|
|
||
| // fetch issue relations | ||
| this.rootIssueDetailStore.relation.fetchRelations(workspaceSlug, projectId, issueId); | ||
|
|
||
| // fetching states | ||
| // TODO: check if this function is required | ||
| this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId); | ||
|
|
||
| return issue; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce code duplication.
The fetchIssueWithIdentifier method has significant code duplication with the existing fetchIssue method. Consider extracting common functionality into shared helper methods.
Here's a suggested refactor:
+private handleIssueRelatedData = (issue: TIssue) => {
+ if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions);
+ if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link);
+ if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments);
+ this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed);
+};
+
+private fetchIssueRelatedData = async (workspaceSlug: string, projectId: string, issueId: string) => {
+ await Promise.all([
+ this.rootIssueDetailStore.activity.fetchActivities(workspaceSlug, projectId, issueId),
+ this.rootIssueDetailStore.comment.fetchComments(workspaceSlug, projectId, issueId),
+ this.rootIssueDetailStore.subIssues.fetchSubIssues(workspaceSlug, projectId, issueId),
+ this.rootIssueDetailStore.relation.fetchRelations(workspaceSlug, projectId, issueId),
+ ]);
+};
+
fetchIssueWithIdentifier = async (workspaceSlug: string, project_identifier: string, sequence_id: string) => {
const query = {
expand: "issue_reactions,issue_attachments,issue_link,parent",
};
const issue = await this.issueService.retrieveWithIdentifier(workspaceSlug, project_identifier, sequence_id, query);
const issueIdentifier = `${project_identifier}-${sequence_id}`;
const issueId = issue?.id;
const projectId = issue?.project_id;
if (!issue || !projectId || !issueId) throw new Error("Issue not found");
const issuePayload = this.addIssueToStore(issue);
this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]);
// handle parent issue if exists
if (issue?.parent && issue?.parent?.id && issue?.parent?.project_id) {
this.issueService.retrieve(workspaceSlug, issue.parent.project_id, issue.parent.id).then((res) => {
this.rootIssueDetailStore.rootIssueStore.issues.addIssue([res]);
});
}
// add identifiers to map
this.rootIssueDetailStore.rootIssueStore.issues.addIssueIdentifier(issueIdentifier, issueId);
- // add related data
- if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions);
- if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link);
- if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments);
- this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed);
+ this.handleIssueRelatedData(issue);
- // fetch related data
- // issue reactions
- if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issueId, issue.issue_reactions);
-
- // fetch issue links
- if (issue.issue_link) this.rootIssueDetailStore.addLinks(issueId, issue.issue_link);
-
- // fetch issue attachments
- if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issueId, issue.issue_attachments);
-
- this.rootIssueDetailStore.addSubscription(issueId, issue.is_subscribed);
-
- // fetch issue activity
- this.rootIssueDetailStore.activity.fetchActivities(workspaceSlug, projectId, issueId);
-
- // fetch issue comments
- this.rootIssueDetailStore.comment.fetchComments(workspaceSlug, projectId, issueId);
-
- // fetch sub issues
- this.rootIssueDetailStore.subIssues.fetchSubIssues(workspaceSlug, projectId, issueId);
-
- // fetch issue relations
- this.rootIssueDetailStore.relation.fetchRelations(workspaceSlug, projectId, issueId);
+ await this.fetchIssueRelatedData(workspaceSlug, projectId, issueId);
// fetching states
// TODO: check if this function is required
this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId);
return issue;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fetchIssueWithIdentifier = async (workspaceSlug: string, project_identifier: string, sequence_id: string) => { | |
| const query = { | |
| expand: "issue_reactions,issue_attachments,issue_link,parent", | |
| }; | |
| const issue = await this.issueService.retrieveWithIdentifier(workspaceSlug, project_identifier, sequence_id, query); | |
| const issueIdentifier = `${project_identifier}-${sequence_id}`; | |
| const issueId = issue?.id; | |
| const projectId = issue?.project_id; | |
| if (!issue || !projectId || !issueId) throw new Error("Issue not found"); | |
| const issuePayload = this.addIssueToStore(issue); | |
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]); | |
| // handle parent issue if exists | |
| if (issue?.parent && issue?.parent?.id && issue?.parent?.project_id) { | |
| this.issueService.retrieve(workspaceSlug, issue.parent.project_id, issue.parent.id).then((res) => { | |
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([res]); | |
| }); | |
| } | |
| // add identifiers to map | |
| this.rootIssueDetailStore.rootIssueStore.issues.addIssueIdentifier(issueIdentifier, issueId); | |
| // add related data | |
| if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions); | |
| if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link); | |
| if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments); | |
| this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed); | |
| // fetch related data | |
| // issue reactions | |
| if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issueId, issue.issue_reactions); | |
| // fetch issue links | |
| if (issue.issue_link) this.rootIssueDetailStore.addLinks(issueId, issue.issue_link); | |
| // fetch issue attachments | |
| if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issueId, issue.issue_attachments); | |
| this.rootIssueDetailStore.addSubscription(issueId, issue.is_subscribed); | |
| // fetch issue activity | |
| this.rootIssueDetailStore.activity.fetchActivities(workspaceSlug, projectId, issueId); | |
| // fetch issue comments | |
| this.rootIssueDetailStore.comment.fetchComments(workspaceSlug, projectId, issueId); | |
| // fetch sub issues | |
| this.rootIssueDetailStore.subIssues.fetchSubIssues(workspaceSlug, projectId, issueId); | |
| // fetch issue relations | |
| this.rootIssueDetailStore.relation.fetchRelations(workspaceSlug, projectId, issueId); | |
| // fetching states | |
| // TODO: check if this function is required | |
| this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId); | |
| return issue; | |
| }; | |
| // Add the new helper methods at the appropriate location within the class | |
| private handleIssueRelatedData = (issue: TIssue) => { | |
| if (issue.issue_reactions) this.rootIssueDetailStore.addReactions(issue.id, issue.issue_reactions); | |
| if (issue.issue_link) this.rootIssueDetailStore.addLinks(issue.id, issue.issue_link); | |
| if (issue.issue_attachments) this.rootIssueDetailStore.addAttachments(issue.id, issue.issue_attachments); | |
| this.rootIssueDetailStore.addSubscription(issue.id, issue.is_subscribed); | |
| }; | |
| private fetchIssueRelatedData = async (workspaceSlug: string, projectId: string, issueId: string) => { | |
| await Promise.all([ | |
| this.rootIssueDetailStore.activity.fetchActivities(workspaceSlug, projectId, issueId), | |
| this.rootIssueDetailStore.comment.fetchComments(workspaceSlug, projectId, issueId), | |
| this.rootIssueDetailStore.subIssues.fetchSubIssues(workspaceSlug, projectId, issueId), | |
| this.rootIssueDetailStore.relation.fetchRelations(workspaceSlug, projectId, issueId), | |
| ]); | |
| }; | |
| fetchIssueWithIdentifier = async (workspaceSlug: string, project_identifier: string, sequence_id: string) => { | |
| const query = { | |
| expand: "issue_reactions,issue_attachments,issue_link,parent", | |
| }; | |
| const issue = await this.issueService.retrieveWithIdentifier(workspaceSlug, project_identifier, sequence_id, query); | |
| const issueIdentifier = `${project_identifier}-${sequence_id}`; | |
| const issueId = issue?.id; | |
| const projectId = issue?.project_id; | |
| if (!issue || !projectId || !issueId) throw new Error("Issue not found"); | |
| const issuePayload = this.addIssueToStore(issue); | |
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]); | |
| // handle parent issue if exists | |
| if (issue?.parent && issue?.parent?.id && issue?.parent?.project_id) { | |
| this.issueService.retrieve(workspaceSlug, issue.parent.project_id, issue.parent.id).then((res) => { | |
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([res]); | |
| }); | |
| } | |
| // add identifiers to map | |
| this.rootIssueDetailStore.rootIssueStore.issues.addIssueIdentifier(issueIdentifier, issueId); | |
| this.handleIssueRelatedData(issue); | |
| await this.fetchIssueRelatedData(workspaceSlug, projectId, issueId); | |
| // fetching states | |
| // TODO: check if this function is required | |
| this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId); | |
| return issue; | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 311-311: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const redirectToBrowseUrl = async () => { | ||
| if (!workspaceSlug || !projectId || !issueId) return; | ||
| try { | ||
| const meta = await issueService.getIssueMetaFromURL( | ||
| workspaceSlug.toString(), | ||
| projectId.toString(), | ||
| issueId.toString() | ||
| ); | ||
| router.push(`/${workspaceSlug}/browse/${meta.project_identifier}-${meta.sequence_id}`); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
| }; | ||
| window.addEventListener("resize", handleToggleIssueDetailSidebar); | ||
| handleToggleIssueDetailSidebar(); | ||
| return () => window.removeEventListener("resize", handleToggleIssueDetailSidebar); | ||
| }, [issueDetailSidebarCollapsed, toggleIssueDetailSidebar]); | ||
|
|
||
| redirectToBrowseUrl(); | ||
| }, [workspaceSlug, projectId, issueId, router]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and user feedback.
Currently, errors are only logged to console, leaving the user on a spinner page with no explanation. Consider adding UI feedback or redirection for failures (e.g., a 404 page or an error alert). Double-check whether .toString() on workspaceSlug, projectId, and issueId is necessary if they are already strings. Also, decide if router.replace might be more suitable than router.push to prevent back-navigation to the invalid URL.
For example:
try {
const meta = await issueService.getIssueMetaFromURL(
workspaceSlug.toString(),
projectId.toString(),
issueId.toString()
);
router.push(`/${workspaceSlug}/browse/${meta.project_identifier}-${meta.sequence_id}`);
} catch (error) {
- console.error(error);
+ // Provide user-friendly messaging or 404 handling
+ console.error("Failed to retrieve issue metadata:", error);
+ // e.g., show a modal or redirect to an error page
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const redirectToBrowseUrl = async () => { | |
| if (!workspaceSlug || !projectId || !issueId) return; | |
| try { | |
| const meta = await issueService.getIssueMetaFromURL( | |
| workspaceSlug.toString(), | |
| projectId.toString(), | |
| issueId.toString() | |
| ); | |
| router.push(`/${workspaceSlug}/browse/${meta.project_identifier}-${meta.sequence_id}`); | |
| } catch (error) { | |
| console.error(error); | |
| } | |
| }; | |
| window.addEventListener("resize", handleToggleIssueDetailSidebar); | |
| handleToggleIssueDetailSidebar(); | |
| return () => window.removeEventListener("resize", handleToggleIssueDetailSidebar); | |
| }, [issueDetailSidebarCollapsed, toggleIssueDetailSidebar]); | |
| redirectToBrowseUrl(); | |
| }, [workspaceSlug, projectId, issueId, router]); | |
| const redirectToBrowseUrl = async () => { | |
| if (!workspaceSlug || !projectId || !issueId) return; | |
| try { | |
| const meta = await issueService.getIssueMetaFromURL( | |
| workspaceSlug.toString(), | |
| projectId.toString(), | |
| issueId.toString() | |
| ); | |
| router.push(`/${workspaceSlug}/browse/${meta.project_identifier}-${meta.sequence_id}`); | |
| } catch (error) { | |
| // Provide user-friendly messaging or 404 handling | |
| console.error("Failed to retrieve issue metadata:", error); | |
| // e.g., show a modal or redirect to an error page | |
| } | |
| }; | |
| redirectToBrowseUrl(); | |
| }, [workspaceSlug, projectId, issueId, router]); |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
web/core/components/issues/sub-issues/root.tsx (1)
404-404: Fix the disabled prop value.The
disabledprop is being negated when passed to theIssueListcomponent, which might lead to unexpected behavior.- disabled={!disabled} + disabled={disabled}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/issues/sub-issues/issue-list-item.tsx(3 hunks)web/core/components/issues/sub-issues/root.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/sub-issues/issue-list-item.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-web
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
web/core/components/issues/sub-issues/root.tsx (1)
301-308: Verify the cleanup logic in useEffect.The cleanup function is calling
handleFetchSubIssueswhich triggers a new fetch operation. This might lead to unnecessary API calls during component unmount.Consider if the cleanup function should instead reset the visibility state:
useEffect(() => { handleFetchSubIssues(); return () => { - handleFetchSubIssues(); + setSubIssueHelpers(`${parentIssueId}_root`, "issue_visibility", ""); }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [parentIssueId]);
| const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | ||
| copyTextToClipboard(`${originURL}/${text}`).then(() => { | ||
| copyTextToClipboard(`${originURL}${text}`).then(() => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the generateWorkItemLink helper function for URL construction.
The current implementation directly concatenates URLs, which may not align with the new URL pattern structure introduced in this PR. Consider using the new generateWorkItemLink helper function to ensure consistent URL construction across the application.
Import and use the helper function:
+import { generateWorkItemLink } from "@/helpers/issue.helper";
-copyTextToClipboard(`${originURL}${text}`).then(() => {
+copyTextToClipboard(generateWorkItemLink(workspaceSlug, projectId, text)).then(() => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | |
| copyTextToClipboard(`${originURL}/${text}`).then(() => { | |
| copyTextToClipboard(`${originURL}${text}`).then(() => { | |
| import { generateWorkItemLink } from "@/helpers/issue.helper"; | |
| const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | |
| copyTextToClipboard(generateWorkItemLink(workspaceSlug, projectId, text)).then(() => { |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx (1)
148-152: Fix inconsistent props in IssueIdentifier component.The IssueIdentifier component for blocked by issues uses different props compared to other instances. It should use
projectIdentifierandissueSequenceIdfor consistency.Apply this diff:
- <IssueIdentifier - issueId={blockedByIssues[0]?.id} - projectId={blockedByIssueProjectDetails?.id} - textContainerClassName="text-xs text-custom-text-200 font-medium" - /> + <IssueIdentifier + projectIdentifier={blockedByIssueProjectDetails?.identifier} + projectId={blockedByIssueProjectDetails?.id} + issueSequenceId={blockedByIssues[0]?.sequence_id} + textContainerClassName="text-xs text-custom-text-200 font-medium" + />
🧹 Nitpick comments (3)
web/core/components/issues/issue-detail/issue-activity/activity/actions/helpers/issue-link.tsx (1)
40-40: Simplify the href attribute.The ternary operation can be simplified since
workItemLinkalready contains the complete URL.- href={`${activity.issue_detail ? workItemLink : "#"}`} + href={activity.issue_detail ? workItemLink : "#"}web/core/components/issues/issue-detail/relation-select.tsx (2)
119-125: Add explicit null check for projectDetails.Consider adding an explicit null check for
projectDetailsbefore using it in the link generation.+ if (!projectDetails) return null; href={generateWorkItemLink({ workspaceSlug, projectId: projectDetails?.id, issueId: currentIssue.id, projectIdentifier: projectDetails?.identifier, sequenceId: currentIssue?.sequence_id, })}
137-141: Consider extracting the event handler.The inline event handler could be extracted to a separate function for better readability and reusability.
+ const handleRemoveRelation = (e: React.MouseEvent) => { + e.preventDefault(); + e.stopPropagation(); + removeRelation(workspaceSlug, projectId, issueId, relationKey, relationIssueId); + }; onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - removeRelation(workspaceSlug, projectId, issueId, relationKey, relationIssueId); + handleRemoveRelation(e); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/types/src/issues/activity/base.d.ts(1 hunks)web/core/components/command-palette/helpers.tsx(2 hunks)web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx(7 hunks)web/core/components/issues/issue-detail/issue-activity/activity/actions/helpers/issue-link.tsx(2 hunks)web/core/components/issues/issue-detail/parent/sibling-item.tsx(2 hunks)web/core/components/issues/issue-detail/relation-select.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
packages/types/src/issues/activity/base.d.ts (1)
29-29: LGTM! Type change aligns with new URL pattern requirements.Changing
sequence_idfrom boolean to number is appropriate for representing sequential identifiers in the new URL format:/{workspace_slug}/browse/{project_identifier}-{issue_sequence}.web/core/components/issues/issue-detail/parent/sibling-item.tsx (1)
34-40: Add error handling for missing project details.The
workItemLinkgeneration assumesprojectDetailsexists. Consider adding a null check or early return if project details are missing.web/core/components/command-palette/helpers.tsx (1)
53-59: LGTM! Clean implementation of the new URL pattern.The implementation correctly uses the
generateWorkItemLinkhelper function with all required parameters from the issue object.web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx (7)
12-12: LGTM!The import statement is correctly placed and aligns with the PR objectives.
45-51: LGTM!The implementation correctly uses the new helper function with all required parameters and proper null handling.
Also applies to: 55-55
113-119: LGTM!The implementation correctly uses the new helper function with all required parameters and proper null handling.
Also applies to: 123-123
174-180: LGTM!The implementation correctly uses the new helper function with all required parameters and proper null handling.
Also applies to: 184-184
221-227: LGTM!The implementation correctly uses the new helper function with all required parameters and proper null handling.
Also applies to: 231-231
289-295: LGTM!The implementation correctly uses the new helper function with all required parameters and proper null handling.
Also applies to: 299-299
355-361: LGTM!The implementation correctly uses the new helper function with all required parameters and proper null handling.
Also applies to: 365-365
| @@ -26,7 +26,7 @@ export type TIssueActivityProjectDetail = { | |||
|
|
|||
| export type TIssueActivityIssueDetail = { | |||
| id: string; | |||
| sequence_id: boolean; | |||
| sequence_id: number; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify impact of type change across the codebase.
This is a breaking change that requires updates to any code using the TIssueActivityIssueDetail type.
Let's verify the impact:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of TIssueActivityIssueDetail and sequence_id to identify potential breaking changes
# Find files importing or using TIssueActivityIssueDetail
echo "Files potentially affected by the type change:"
rg "TIssueActivityIssueDetail"
# Find direct uses of sequence_id property
echo -e "\nDirect uses of sequence_id:"
rg "sequence_id" --type ts --type tsxLength of output: 719
🏁 Script executed:
#!/bin/bash
# Re-run the search for direct uses of the sequence_id property across TypeScript, TSX, and declaration files using glob patterns instead of file type flags
echo "Direct uses of sequence_id across *.d.ts, *.ts, and *.tsx files:"
rg sequence_id -g "*.d.ts" -g "*.ts" -g "*.tsx"Length of output: 13269
Impact of Changing sequence_id Type in TIssueActivityIssueDetail
Our investigation shows that the updated type in
packages/types/src/issues/activity/base.d.ts
declares sequence_id as a number. However, multiple parts of the codebase reference sequence_id—and in several declarations (e.g., in packages/types/src/inbox.d.ts and in web/core/services/issue/issue.service.ts) it is defined or used as a string. This discrepancy indicates that changing the type here is indeed a breaking change that will affect any consumers of TIssueActivityIssueDetail relying on a string type.
- Affected areas:
- Files where
TIssueActivityIssueDetailis imported (e.g., issue activity and issue comment types). - Modules that explicitly type
sequence_idas a string.
- Files where
Please review and update all consumers of TIssueActivityIssueDetail to accommodate the new type (or adjust the type change if unintended).
| const workItemLink = generateWorkItemLink({ | ||
| workspaceSlug: activity.workspace_detail?.slug, | ||
| projectId: activity.project, | ||
| issueId: activity.issue, | ||
| projectIdentifier: activity.project_detail.identifier, | ||
| sequenceId: activity.issue_detail.sequence_id, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add null checks for required properties.
The generateWorkItemLink function might receive undefined values which could lead to malformed URLs. Consider adding null checks or providing default values.
🏁 Script executed:
#!/bin/bash
# Search for the implementation of generateWorkItemLink to verify required parameters
ast-grep --pattern 'function generateWorkItemLink($_) {
$$$
}'Length of output: 66
Let's gather additional data. I'll run new shell scripts to locate the definition of generateWorkItemLink and inspect its usage for potential null-checks.
🏁 Script executed:
#!/bin/bash
# Locate the definition(s) of generateWorkItemLink in the codebase
rg -n "function\s\+generateWorkItemLink" .
# Inspect the file where generateWorkItemLink is invoked to check if there’s any surrounding null-check logic
rg -n "generateWorkItemLink" web/core/components/issues/issue-detail/issue-activity/activity/actions/helpers/issue-link.tsxLength of output: 282
Let's gather more details by locating the implementation of generateWorkItemLink. We'll search for files where it's defined (likely as an exported constant) and inspect if it already handles null/undefined cases.
🏁 Script executed:
#!/bin/bash
# Locate files that might define generateWorkItemLink
fd --type f -e ts "issue.helper"
# Search for the function definition using an export pattern
rg -n "export const generateWorkItemLink" .Length of output: 209
Let's inspect the implementation details of generateWorkItemLink to see if it already performs null checks before constructing the URL.
🏁 Script executed:
#!/bin/bash
# Display the implementation of generateWorkItemLink to inspect null-checks and default values.
sed -n '320,400p' web/helpers/issue.helper.tsLength of output: 1162
Null-checks Missing in URL Construction
The implementation of generateWorkItemLink directly interpolates parameters into URL strings without verifying if any of the required properties (e.g., workspaceSlug, projectId, issueId, projectIdentifier, or sequenceId) are null or undefined. This may lead to malformed URLs if any of these values are missing.
- The function accepts values as
string | undefined | nullbut does not include any checks or fallback defaults. - The calling code doesn’t validate these properties before passing them to
generateWorkItemLink.
It is recommended to add proper null checks or default values either within generateWorkItemLink or before invoking it to ensure the generated URLs are always valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/core/components/workspace/sidebar/project-navigation.tsx (1)
157-161: LGTM! Consider extracting the URL pattern to a constant.The logic correctly handles both the traditional and new URL patterns for issues, aligning with the PR's objective of introducing a shorter URL format.
Consider extracting the URL pattern to a constant for better maintainability:
+const BROWSE_PATH_PATTERN = "browse"; + <SidebarNavItem isActive={ item.key === "issues" - ? pathname.includes(item.href) || pathname.includes(`/${workspaceSlug}/browse/`) + ? pathname.includes(item.href) || pathname.includes(`/${workspaceSlug}/${BROWSE_PATH_PATTERN}/`) : pathname.includes(item.href) } >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx(1 hunks)web/core/components/workspace/sidebar/project-navigation.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/app/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/core/components/issues/issue-detail/parent/root.tsx (1)
54-66: Consider adding error handling for invalid parent issues.The link generation and click handling logic is well-implemented. However, consider adding error handling for cases where
projectIdentifiermight be undefined.const workItemLink = generateWorkItemLink({ workspaceSlug, projectId: parentIssue?.project_id, issueId: parentIssue.id, - projectIdentifier, + projectIdentifier: projectIdentifier ?? parentIssue?.project_id, sequenceId: parentIssue.sequence_id, isEpic: isParentEpic, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/core/components/issues/issue-detail/parent/root.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
web/core/components/issues/issue-detail/parent/root.tsx (3)
5-5: LGTM! Imports are well-organized and align with the PR objectives.The new imports support the URL pattern changes and link generation functionality.
Also applies to: 11-14
33-46: LGTM! Derived values are well-structured and follow React best practices.The router initialization and derived values are properly organized, improving code readability and maintainability.
71-71: LGTM! UI changes maintain consistency and accessibility.The integration of the new link generation and click handling is clean and maintains the component's accessibility.
Description
This PR introduces several enhancements and improvements:
Shorter URL for Work Items:
Users can now access work items using a shorter URL pattern:
/{workspace_slug}/browse/{project_identifier}-{issue_sequence}
If a user visits the old URL, they will be redirected to the new one.
generateWorkItemLink Helper Function:
A new helper function generateWorkItemLink is introduced to construct work item links consistently.
Work Item Redirection and Code Refactor:
Updated work item redirection logic and performed code refactoring to improve readability and maintainability.
Type of Change
Test Scenarios
Affected Areas
References
[WEB-3268]
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor